Skip to content

Fill response flag for stats plugin#2527

Merged
istio-testing merged 5 commits intoistio:masterfrom
bianpengyuan:response-flag
Nov 7, 2019
Merged

Fill response flag for stats plugin#2527
istio-testing merged 5 commits intoistio:masterfrom
bianpengyuan:response-flag

Conversation

@bianpengyuan
Copy link
Contributor

@bianpengyuan bianpengyuan commented Nov 6, 2019

This needs istio/envoy#116 to be in to work with 1.4 and envoyproxy/envoy-wasm to be updated to work with master.

@bianpengyuan bianpengyuan requested review from a team, kyessenov and mandarjog November 6, 2019 01:36
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 6, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 6, 2019
kyessenov
kyessenov reviewed Nov 6, 2019
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. The only suggestion I have is that unrecognized bits should be supplied since this may need to work with future Envoy versions. E.g. drop LastFlag and just print out the flags as-is in case they overflow.

@bianpengyuan bianpengyuan mentioned this pull request Nov 6, 2019
Merged
@bianpengyuan bianpengyuan added the cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch label Nov 6, 2019
@bianpengyuan
Copy link
Contributor Author

bianpengyuan commented Nov 7, 2019

Can I get some review on this? Thanks!

@bianpengyuan
Copy link
Contributor Author

bianpengyuan commented Nov 7, 2019 •
edited
Loading

@kyessenov Does this look like a flake to you? It seems to be different from client timeout we saw before. There is no crashes and does not return any mismatches.. so likely the prometheus endpoint does not return metrics. Could this also happen because of stale listeners?

--- FAIL: TestStatsParallel (48.68s)
    stats_xds_test.go:233: failed to match all stats
FAIL
FAIL	istio.io/proxy/test/envoye2e/stats	66.504s
ok  	istio.io/proxy/test/envoye2e/stats_plugin	23.067s
ok  	istio.io/proxy/test/envoye2e/tcp_metadata_exchange	5.045s
Makefile.core.mk:79: recipe for target 'test_tsan' failed
make: *** [test_tsan] Error 1

@kyessenov
Copy link
Contributor

kyessenov commented Nov 7, 2019

@bianpengyuan Yeah this is not a timeout. Either prometheus reports incorrect stats, or we failed to report one. Does this happen consistently? We can disable the test for now to get this in.

@bianpengyuan
Copy link
Contributor Author

bianpengyuan commented Nov 7, 2019

@kyessenov haven't retried it. Let me retest it.

@bianpengyuan
Copy link
Contributor Author

bianpengyuan commented Nov 7, 2019

/test proxy-presubmit-tsan

@istio-testing istio-testing merged commit c3fda9c into istio:master Nov 7, 2019
@istio-testing istio-testing mentioned this pull request Nov 7, 2019
Merged
@istio-testing
Copy link
Collaborator

istio-testing commented Nov 7, 2019

In response to a cherrypick label: new pull request created: #2534

@kyessenov
Copy link
Contributor

kyessenov commented Nov 7, 2019

I noticed that it was polling stats endpoint for ~20s. I think it was just starved of CPU resources since TSAN builds are slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyessenov kyessenov kyessenov approved these changes

@mandarjog mandarjog Awaiting requested review from mandarjog

Assignees

No one assigned

Labels

cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bianpengyuan @kyessenov @istio-testing @googlebot